Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Raised for testing purpose - Contains all code of GSoC 2024 #5946

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft

Raised for testing purpose - Contains all code of GSoC 2024 #5946

wants to merge 24 commits into from

Conversation

omChauhanDev
Copy link
Contributor

What this PR does

This pr is created for testing all training modification features.
Eg: how they work over production server

@ragesoss
Copy link
Member

ragesoss commented Sep 6, 2024

This is an impressive set of functionality.

I've done some initial testing, and there are a few UX problems that I think will need to be fixed before this is ready to deploy.

  • There needs to be a way to edit an existing slide. Since we are not supporting slide deletion, it's important to be able to change the wiki page for an existing slide, so that misconfiguring a new slide will not render the slide slug unusable.
  • The wiki page field should automatically handle input that is a meta.wikimedia.org full URL instead of just a page name. Copying and pasting the full URL from a wiki is a common pattern in the dashboard, and it's not obvious whether a user ought to use a URL or just a page title.

@ragesoss
Copy link
Member

ragesoss commented Sep 6, 2024

I also encountered a JS error when entering 'reorder' mode on an existing module's index page:

TypeError: Cannot read properties of null (reading 'split')
    at DraggableSlide (draggableSlide.jsx:68:1)
    at renderWithHooks (react-dom.development.js:16305:18)
    at mountIndeterminateComponent (react-dom.development.js:20069:13)
    at beginWork (react-dom.development.js:21582:16)
    at beginWork$1 (react-dom.development.js:27421:14)
    at performUnitOfWork (react-dom.development.js:26552:12)
    at workLoopSync (react-dom.development.js:26461:5)
    at renderRootSync (react-dom.development.js:26429:7)
    at recoverFromConcurrentError (react-dom.development.js:25845:20)
    at performSyncWorkOnRoot (react-dom.development.js:26091:20)
    at flushSyncCallbacks (react-dom.development.js:12042:22)
    at eval (react-dom.development.js:25646:13)

@ragesoss ragesoss marked this pull request as draft September 6, 2024 19:53
@omChauhanDev
Copy link
Contributor Author

Fixing this error and also reducing some unnecessary lines of code of 'reordering slides' pr. Then will move on to incorporating the listed changes to make it perfect for deployment.

@omChauhanDev
Copy link
Contributor Author

omChauhanDev commented Sep 17, 2024 •
edited
Loading

sir, where you found this error as, I tried replicating this error in my browser console but found nothing
kindly have a look : Video

@omChauhanDev omChauhanDev marked this pull request as ready for review September 18, 2024 10:47
@omChauhanDev omChauhanDev marked this pull request as draft September 18, 2024 11:18
@omChauhanDev
Copy link
Contributor Author

@ragesoss sir, kindly have a look

@ragesoss
Copy link
Member

Hmm... I still hit that same error. I'm doing this in wiki_education mode, with freshly reloaded training modules, and any time I click 'Change Order' on the index page of one of the YAML-backed modules, it throws a JS error and the React-rendered sidebar disappears.

ragesoss
ragesoss reviewed Oct 24, 2024
View reviewed changes
app/assets/javascripts/training/components/draggableSlide.jsx Outdated
<div className="draggable-slide-container">
<div className="program-description__header">
<h4><strong>{heading}</strong></h4>
{description.split('\n').map((paragraph, i) => paragraph && <p key={i}>{paragraph}</p>)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where it errors when I enter reorder mode for a yml-based module. description comes from slide.wiki_page, so when the wiki_page is null, it will error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a null check there.

@omChauhanDev
Added functionality for creating new Training Libraries
a8481d6
@omChauhanDev
Added rspec test cases
3a0ea9c
@omChauhanDev
Changed the validation pattern
8003ee1
@omChauhanDev
Added more scoping for styles
e553d8c
@omChauhanDev
Developed functionality of adding categories to training libraries
0fff88b
@omChauhanDev
Added functionality of deleting category which are having no modules
105509f
@omChauhanDev
Created rspec tests for adding and removing category in Training Library
53fa746
@omChauhanDev
Improved: Refreshing the page, adjustedtest cases accordingly
a90e2bf
@omChauhanDev
Added Functionality to Switch between Edit and View Mode for Training…
60c10b7 
… Content
@omChauhanDev
refactored the code: reduced backend calls
1503912
@omChauhanDev
created-functionality-of-adding-new-training-module
bfdfb20
@omChauhanDev
added feature of transferring modules between categories and created …
fc37eca 
…its rspec test
@omChauhanDev
Simplified code and removed redundancy
aa1da72
@omChauhanDev
modified approach: using #react_root instead of parsing data from html
743c8fe
@omChauhanDev
removed unnecessary function
4758364
@omChauhanDev
developed functionality for adding slides to training module
3c22705
@omChauhanDev
added test cases for adding slides to training module
30a3998
@omChauhanDev
added functionality for removing training slides
56c69af
@omChauhanDev
added test case for removing training slide
d337e6f
@omChauhanDev
added functionality for reordering training slides
2ee8287
@omChauhanDev omChauhanDev force-pushed the feature/ordering-slides-of-training-module branch from 596a1a4 to 9125c59 Compare November 6, 2024 20:42
@omChauhanDev
Copy link
Contributor Author

@ragesoss sir, kindly verify that js error that we have encountered earlier is not happening now ?

@ragesoss
Copy link
Member

ragesoss commented Nov 7, 2024

Yep! That fixes the error. Thanks!

I will continue testing from here, this week or next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ragesoss ragesoss ragesoss left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@omChauhanDev @ragesoss